fix(router): preserve Windows literal path payloads through skill dispatch#481
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
b5dcc1afor PR #481
Review record:
10f72bd8-6035-4e4a-9c1c-ab56f2d5dbed
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/router/dispatch.py:328 | BLOCKING | The new multiline remainder support does not round-trip through the public ParsedOooCommand entrypoint. resolve_skill_dispatch(parsed, ...) and resolve_parsed_skill_dispatch(parsed, prompt=None, ...) reconstruct Resolved.prompt with f"{parsed.command_prefix} {parsed.remainder}", which inserts a space even when the original separator was a newline. For multiline inline seed content, callers that pass a parsed command now get a different prompt than the one they parsed, losing the exact payload layout that this PR is trying to preserve. |
Non-blocking Suggestions
| 1 | tests/unit/router/test_valid_dispatch_normalization.py:151 | tests | Add a regression test that resolves a multiline ParsedOooCommand directly, not just a raw prompt string. That is the public path currently missing coverage, and it would catch the prompt reconstruction issue above. |
Design Notes
The change is narrowly targeted and the parser/extractor split still makes sense, but the new multiline capability is only wired through the raw-prompt flow. The parsed-command API needs the same exact-text preservation guarantee to keep the router’s public contract consistent.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
The shared router parses ``ParsedOooCommand`` from a raw prompt and exposes a
public API that accepts the parsed object directly. The previous
``_reconstruct_prompt_from_parsed_command`` helper always rejoined the
command prefix and remainder with a single space, which silently collapsed
multiline ``/ouroboros:run`` payloads and other whitespace-sensitive inputs
when callers used the parsed-command entrypoint instead of the raw-prompt
entrypoint.
This change carries the original separator on ``ParsedOooCommand`` so prompt
reconstruction roundtrips exactly. The new field is excluded from equality
comparisons (``compare=False``) so existing parser test fixtures that
construct ``ParsedOooCommand`` without a separator continue to compare
correctly against parser output.
- ``router.types.ParsedOooCommand`` gains ``separator: str = " "``
(``compare=False``)
- ``router.command_parser`` captures the separator named group from the
shared regex and forwards it to ``ParsedOooCommand``
- ``router.dispatch._reconstruct_prompt_from_parsed_command`` uses the
carried separator; ``_validate_parsed_command`` checks that ``separator``
is a string for symmetry with the other field validators
- regression tests
- parser: separator capture across space, tab, multiline, and trailing
whitespace inputs, plus default fallback for absent remainder
- dispatch: parsed-command-path roundtrip for multiline inline seed
content, Windows literal paths, and externally constructed
``ParsedOooCommand`` defaulting to space separator
Addresses the in-PR review feedback on Q00#481 about the parsed-command
public API path losing newline separators when the raw-prompt path was
already preserving them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
0eb2cc8for PR #481
Review record:
48214870-28a8-4838-9c92-3eb843325dec
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/router/dispatch.py:276 | BLOCKING | The new multiline/Windows fast path calls remainder.strip() before returning it, which silently drops leading indentation and trailing newlines from inline payloads. That contradicts the stated goal of preserving /ouroboros:run\n<yaml> content verbatim, and it can corrupt whitespace-sensitive seed/YAML input before $1 substitution. For example, /ouroboros:run\n goal: test\n will be dispatched as goal: test, not the original payload. |
Non-blocking Suggestions
None.
Design Notes
The separator-roundtripping change is directionally correct and keeps the parsed-command API aligned with the raw-prompt path, but the payload-preservation logic should avoid normalizing the payload itself. Preserve the separator separately; do not mutate remainder when deriving first_argument for whitespace-sensitive inputs.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Review response✅ Blocking — FIXED in commit
|
…paths The previous fast-path branches in ``extract_first_argument`` returned ``remainder.strip()`` for both multiline and Windows-literal payloads, which silently dropped leading indentation and trailing newlines. That contradicts the goal of preserving inline ``/ouroboros:run\n<yaml>`` content verbatim; ``/ouroboros:run\n goal: test\n`` would have been dispatched as ``goal: test`` instead of the original payload. This commit returns the original ``remainder`` value in both fast-path branches so whitespace-sensitive seed/YAML inputs reach the MCP tool with their layout intact. Windows-literal detection still tolerates incidental leading whitespace by matching against ``remainder.strip()`` for classification while returning the unstripped value, so existing single-line Windows tests continue to behave identically. - ``router.dispatch.extract_first_argument`` returns ``remainder`` (not the stripped form) for both multiline and Windows-literal payloads - regression coverage in ``tests/unit/router/test_extract_first_argument.py`` for leading indentation, trailing newlines, leading + trailing whitespace combos, and Windows literal paths with incidental leading whitespace Addresses the second bot review on PR Q00#481 about payload normalization in the multiline/Windows fast paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review response (2nd round)✅ Blocking — FIXED in commit
|
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
5112c52for PR #481
Review record:
fb7a93df-e319-479b-b9af-67e67ed18b92
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/router/command_parser.py:12 | BLOCKING | The new separator capture is greedy, so for multiline payloads that intentionally begin with indentation, that indentation is consumed into separator instead of staying in remainder. Example: parsing "/ouroboros:run\n goal: test\n" yields separator == "\n " and remainder == "goal: test\n", so extract_first_argument(parsed.remainder) silently drops the leading two spaces. Resolved.prompt roundtrips correctly, but the actual dispatched $1 is wrong for whitespace-sensitive payloads like indented YAML. This is an end-to-end data corruption bug on the parsed/raw dispatch path and needs a parser fix plus a regression test that goes through parse_ooo_command into resolve_skill_dispatch. |
Non-blocking Suggestions
None.
Design Notes
The patch is directionally correct: preserving separator state separately is the right shape, and the extract_first_argument fast paths are narrowly targeted. The remaining issue is in the parser boundary, where separator capture currently mutates the semantic payload for indented multiline inputs.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
…ndent The shared command-parser regex used a greedy ``\s+`` for the separator named group with ``re.DOTALL``, which crossed the newline that ended the command line and consumed the next line's leading indentation. For an inline ``/ouroboros:run\n goal: test\n`` payload that produced ``separator == "\n "`` and ``remainder == "goal: test\n"``, so even though ``Resolved.prompt`` reconstructed identically through the carried separator, ``extract_first_argument(parsed.remainder)`` silently dropped the YAML's leading two-space indent before ``$1`` substitution. The bug only surfaced when both ends of the parser/dispatch boundary were exercised together, which is why the round-2 fix in commit 5112c52 looked sufficient in isolation. This commit narrows the separator pattern to either same-line spaces and tabs optionally terminated by a single ``\r?\n``, or same-line spaces and tabs alone. The newline that ends the command line still becomes the separator, but any leading whitespace on the next line stays attached to ``remainder`` so whitespace-sensitive payloads reach the MCP tool with their layout intact. All previously asserted separator captures (single-space, tab, plain newline, trailing-spaces-only) continue to hold because the new alternation still captures the same canonical shapes for those inputs. - ``router.command_parser`` bounds the separator alternation so leading indent on the line after the command prefix is never absorbed - regression for the parser-level shape: indented multiline payload preserves both the leading-indent ``remainder`` and the canonical ``"\n"`` separator - regression for the dispatch-level shape (raw-prompt and parsed-command entrypoints): ``$1`` and the resolved ``mcp_args`` carry the verbatim indented YAML the caller passed in Addresses the third bot review on PR Q00#481 (commit 5112c52) about the parser separator capture mutating the semantic payload for indented multiline inputs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review response (3rd round)✅ Blocking — FIXED in commit
|
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
c44b09afor PR #481
Review record:
27a0b168-94c4-4c53-861f-ec21b052cb2a
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
The parser/dispatch split is still clean, and the fixes are scoped tightly to the roundtrip problem they were meant to solve. The added separator field is a reasonable way to preserve prompt fidelity without entangling extract_first_argument with reconstruction concerns.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
…patch `extract_first_argument` falls through to `shlex.split` for any single-line remainder, which on POSIX treats backslash as an escape character and silently strips it. As a result, a Windows drive-letter payload like `C:\temp\seed.yaml --strict` reaches the dispatched MCP tool as `C:tempseed.yaml --strict`, and a UNC payload like `\\server\share\seed.yaml --strict` becomes `servershareseed.yaml --strict`. That contradicts the documented contract for `seed_file_or_content` and breaks any other skill argument that legitimately contains backslashes on Windows callers. This patch carves out one narrow fast path: if the remainder, after trimming incidental leading whitespace, starts with a drive-letter prefix (`[A-Za-z]:\`) or a UNC prefix (`\\`), return the remainder verbatim and skip shell tokenization entirely. The carve-out runs after the existing multiline branch (`re.search(r"[\r\n].*\S", remainder)`), so multiline payloads continue to use the multiline path and natural-language single-line prompts continue to use shell-style quote stripping. The Windows-literal pattern matches against `remainder.strip()` for classification while returning the unstripped value, so any incidental leading whitespace in the original payload is preserved. The router parser layer is unchanged. - `router.dispatch._WINDOWS_LITERAL_PATH_PATTERN` (new) recognizes drive-letter and UNC prefixes - `router.dispatch.extract_first_argument` returns the verbatim `remainder` for Windows-literal payloads, before falling through to `shlex.split` - regression for `extract_first_argument`: drive-letter, UNC, and drive-letter with incidental leading whitespace - regression for end-to-end dispatch: drive-letter and UNC payloads reach `mcp_args["seed_path"]` and `Resolved.first_argument` with backslashes intact Refs Q00#480. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c44b09a to
657663c
Compare
PR scope narrowed and rebased on
|
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
657663cfor PR #481
Review record:
792d612a-c572-469d-8980-466c3a3e6715
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
The scope is appropriately narrow: the extract_first_argument fast path in src/ouroboros/router/dispatch.py fixes a real shlex.split corruption case for Windows drive-letter and UNC literals without changing the broader dispatch pipeline, and the added unit/end-to-end coverage in tests/unit/router/test_extract_first_argument.py and tests/unit/router/test_valid_dispatch_normalization.py matches the intended contract. I could not run pytest locally because the environment is missing the pytest module.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Summary
C:\…) and UNC (\\server\share\…) literal path payloads throughextract_first_argumentso backslashes survive skill dispatchextract_first_argumentand end-to-endresolve_skill_dispatchlevelsRefs #480.
Problem
extract_first_argumentreturns single-line remainders throughshlex.split, which on POSIX treats backslash as an escape character and silently drops it. So Windows literal path payloads reach the dispatched MCP tool stripped of every backslash:shlex.split(POSIX) result rejoinedC:\temp\seed.yaml --strictC:tempseed.yaml --strict\\server\share\seed.yaml --strictservershareseed.yaml --strictThat contradicts the documented
seed_file_or_contentcontract for/ouroboros:runand breaks any other skill argument that legitimately contains backslashes on Windows callers.What changed
src/ouroboros/router/dispatch.py_WINDOWS_LITERAL_PATH_PATTERNrecognizing the two literal-path shapes ([A-Za-z]:\and\\)extract_first_argumentcarves out a narrow fast path after the existing multiline branch: if the trimmed remainder starts with one of those prefixes, return the verbatimremainderand skip shell tokenization entirelyremainder.strip()while the return value is the unstrippedremainder, so any incidental leading whitespace in the original payload is preservedThe router parser layer is unchanged; this is strictly a
extract_first_argumentcarve-out.Tests
tests/unit/router/test_extract_first_argument.pytests/unit/router/test_valid_dispatch_normalization.pymcp_args["seed_path"]andResolved.first_argumentwith backslashes intactmcp_args["seed_path"]andResolved.first_argumentwith backslashes intactWhy this PR is intentionally narrow
The original PR also covered multiline inline seed content, but that scope has since been resolved on
mainby a separate set of router fixes (bebda68,bb7a456,e8534e0,40d3cae) using a different implementation strategy. To avoid reintroducing churn or competing designs, this PR has been narrowed to the single concern thatmaindoes not yet cover: Windows literal path preservation. The branch was reset on top ofmainand only the Windows-path fast path remains.Verification
PYTHONPATH=src python3 -m pytest tests/unit/router -q→ 247 passedPYTHONPATH=src python3 -m pytest tests/unit/orchestrator -q→ 804 passed, 1 skippedPYTHONPATH=src python3 -m pytest tests/integration/test_codex_skill_smoke.py tests/integration/test_codex_skill_fallback.py -q→ 3 passedpython3 -m ruff check src/ouroboros/router tests/unit/router→ All checks passedNon-goals
main)ouroboros run <seed.yaml>CLI contract